fix: order ui accent before nested config tables - #2698
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe default configuration now documents ChangesUI accent configuration ordering
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The configuration ordering change includes regression coverage for the intended TOML parent-before-child ordering. No remaining merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main.rs (1)
927-934: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the regression test reject duplicate accent documentation.
findreturns the first matching block. If the removed later block is reintroduced,accent < sidebarstill passes. Count the accent documentation marker and assert that it occurs exactly once before checking the ordering.Proposed test improvement
#[test] fn default_config_lists_ui_accent_before_nested_tables() { - let accent = DEFAULT_CONFIG.find("# accent = \"cyan\"").unwrap(); - let sidebar = DEFAULT_CONFIG.find("# [ui.sidebar.agents]").unwrap(); + let accent_marker = "# Accent color for highlights, borders, and navigation UI."; + assert_eq!(DEFAULT_CONFIG.matches(accent_marker).count(), 1); + let accent = DEFAULT_CONFIG.find("# accent = \"cyan\"").unwrap(); + let sidebar = DEFAULT_CONFIG.find("# [ui.sidebar.agents]").unwrap(); assert!(accent < sidebar); }
|
CodeRabbit review 4913662039: agreed, fixed in dfb0ca1. |
|
|
@akbash-bot the implementation review is clean. please rebase onto current master and rerun checks and both review bots. |
|
@ogulcancelik rebased cleanly onto current master ( |
|
@akbash-bot if this is still relevant given the current master head (check that please), rebase and solve main.rs conflicts. Then re-run ci/reviews and deliver a clean PR |
|
@Pimpmuckl still relevant on master Focused regression, formatting, clippy, 3,418 Rust tests, maintenance tests, and integration tests passed with Zig 0.16.0. Local Still blocked updating this existing PR: |
|
Got it, I'll take over |
dfb0ca1 to
c2bb7d3
Compare
|
@greptileai re-review requested on the rebased head (c2bb7d3) now that the main.rs conflict is resolved and CI is green. |
|
Takeover done. Rebased onto current Validation:
CI is green on |
Summary
ui.accentexample before nested[ui.*]tables in--default-configRebased onto current
master(fc44f61a); the only conflict was insrc/main.rs, resolved by keeping master's updated sidebar built-ins list (which added themachinetoken) and placing theaccentexample above it.Checks
just checkpasses after the rebase: formatting, clippy, 2,949 Rust tests, maintenance/UI hot-path/integration-assets/docs contract tests, andcargo build --lockedcargo test default_config_lists_ui_accent_before_nested_tables -- --nocapture--default-config, uncommentedui.accentand[ui.sidebar.spaces]in place, thenherdr config check(config: ok)refs #2697